Skip to content

fix(utils): update existing keys in-place in FifoCache::push#2065

Open
amathxbt wants to merge 2 commits into
0xMiden:nextfrom
amathxbt:fix/fifo-cache-ghost-eviction-entries
Open

fix(utils): update existing keys in-place in FifoCache::push#2065
amathxbt wants to merge 2 commits into
0xMiden:nextfrom
amathxbt:fix/fifo-cache-ghost-eviction-entries

Conversation

@amathxbt
Copy link
Copy Markdown

@amathxbt amathxbt commented May 9, 2026

Summary

FifoCache::push unconditionally appended key to the eviction queue before calling map.insert, even when the key was already present in the cache. This created a ghost eviction entry: the queue length grew beyond the number of live map entries, consuming an eviction slot that had no corresponding map value. When that ghost eventually surfaced as the oldest entry, map.remove found nothing—silently discarding the slot—while the queue shrank by one. The net effect was that the cache's effective unique-entry capacity was reduced by one for every overwrite, and a still-live entry could be prematurely evicted.

Root Cause

// Before (buggy)
pub fn push(&self, key: K, value: V) {
    let mut inner = self.0.lock().expect("fifo cache lock poisoned");
    if inner.eviction.len() >= inner.capacity.get() {
        if let Some(oldest) = inner.eviction.pop_front() {
            inner.map.remove(&oldest);  // removes ghost if key reused
        }
    }
    inner.eviction.push_back(key.clone()); // appended EVEN for existing keys
    inner.map.insert(key, value);
}

With capacity = 2:

  1. push(A, 1) → queue: [A], map: {A:1}
  2. push(A, 2) → queue: [A, A], map: {A:2} ← ghost created, capacity consumed
  3. push(B, 3) → evicts oldest A (ghost); queue: [A, B], map: {A:2, B:3} ← full
  4. push(C, 4) → evicts real A; queue: [B, C], map: {B:3, C:4} ← A lost prematurely

Fix

Check map.contains_key(&key) first. When the key exists, update the value in-place and return immediately, leaving the eviction queue unchanged:

// After (fixed)
if inner.map.contains_key(&key) {
    inner.map.insert(key, value);
    return;
}

Testing

Added two new tests:

  • overwrite_key_updates_value_in_place — verifies that overwriting a key does not consume an extra eviction slot.
  • overwrite_does_not_change_eviction_position — verifies that the overwritten key is still evicted at its original FIFO position when the cache later fills up.

CHANGELOG

Added entry to ## v0.15.0 (TBD) section.

amathxbt added 2 commits May 9, 2026 02:37
When push() was called with an already-present key the previous
implementation unconditionally appended the key to the eviction
queue before calling map.insert(). This created a ghost entry: the
eviction queue length exceeded the number of live map entries,
effectively reducing unique-entry capacity and causing a valid value
to be prematurely dropped when the ghost surfaced as the oldest key.

Fix: check map.contains_key() first and, when the key exists, update
the value in-place without touching the eviction queue.
if inner.map.contains_key(&key) {
inner.map.insert(key, value);
return;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the bug. However the FIFO behaviour is still not quite right with this fix.

Every time an entry is pushed into the FIFO cache, it should be put (or moved) into the back of the eviction queue. This still doesn't happen after this fix.

I think we have two options as to how to implement this (without an O(n) scan of the eviction queue):

  1. Use a linked hash map (requires external crate) instead of the map and vecdeque; or
  2. A tombstone mechanism which prevents prior entries in the eviction queue from removing entries that were pushed multiple times.

@Mirko-von-Leipzig any thoughts / preference?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc this is used for the block/proofs caching for the subscriptions?

Can we not just use a VecDequeue since they should always be sequential by block number?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants